feat(RichTextInput): create new component#6318
Conversation
🦋 Changeset detectedLatest commit: 63e7331 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6318 +/- ##
==========================================
+ Coverage 83.30% 92.44% +9.13%
==========================================
Files 44 540 +496
Lines 683 10234 +9551
Branches 195 3913 +3718
==========================================
+ Hits 569 9461 +8892
- Misses 104 706 +602
- Partials 10 67 +57
... and 489 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
d4b5583 to
8b976b0
Compare
| {...props} | ||
| error={getError( | ||
| { | ||
| label: label ?? ariaLabel ?? name, |
There was a problem hiding this comment.
| label: label ?? ariaLabel ?? name, | |
| label: errorLabel ?? label ?? ariaLabel ?? name, |
errorLabel is a new prop of BaseFieldProps that can be used to customize the error message (see #6241)
| field.onChange(value) | ||
| onChange?.(value as PathValue<TFieldValues, Path<TFieldValues>>) | ||
| }} | ||
| value={field.value ?? ''} |
| ) | ||
| }, | ||
| ], | ||
| title: 'Form/Components/Fields/RichTextEditorField', |
There was a problem hiding this comment.
| title: 'Form/Components/Fields/RichTextEditorField', | |
| title: 'Form/Components/Compositions/RichTextEditorField', |
|
|
||
| export default { | ||
| component: RichTextEditor, | ||
| title: 'UI/Data Entry/RichTextEditor', |
There was a problem hiding this comment.
| title: 'UI/Data Entry/RichTextEditor', | |
| title: 'Compositions/RichTextEditor', |
| style={{ | ||
| minHeight, | ||
| ...(maxHeight ? { maxHeight } : {}), | ||
| }} |
There was a problem hiding this comment.
why not add this in richTextEditorStyle.docRegion paired with assignInlineVars?
| sentiment?: 'danger' | 'success' | 'neutral' | ||
| }) => ( | ||
| <Row gap="1" templateColumns="minmax(0, 1fr) min-content"> | ||
| <div> |
There was a problem hiding this comment.
i don't understand the role of this div
| }) | ||
|
|
||
| export const docRegion = style({ | ||
| lineHeight: 1.5, |
There was a problem hiding this comment.
| lineHeight: 1.5, | |
| lineHeight: theme.typography.body.lineHeight, |
82f8b17 to
82b9a91
Compare
| ) | ||
| }, | ||
| ], | ||
| title: 'Compositions/RichTextEditorField', |
There was a problem hiding this comment.
| title: 'Compositions/RichTextEditorField', | |
| title: 'Form/Components/Compositions/RichTextEditorField', |
| const maxHeight = | ||
| typeof maxRows === 'number' | ||
| ? `calc(${lineHeightEm}em * ${maxRows} + 2 * ${padding})` | ||
| : undefined |
There was a problem hiding this comment.
| const maxHeight = | |
| typeof maxRows === 'number' | |
| ? `calc(${lineHeightEm}em * ${maxRows} + 2 * ${padding})` | |
| : undefined | |
| const maxHeight = | |
| typeof maxRows === 'number' | |
| ? `calc(${lineHeightEm}em * ${maxRows} + 2 * ${padding})` | |
| : 'none' |
Just a suggestion
| ...assignInlineVars({ | ||
| [docRegionMaxHeightVar]: maxHeight ?? 'none', | ||
| }), | ||
| minHeight, |
There was a problem hiding this comment.
minHeight should be an inlineVar too
| const isInOrderedList = isSelectionInNodeType(editorState, orderedList) | ||
|
|
||
| return ( | ||
| <Stack alignItems="center" direction="row" gap={1}> |
There was a problem hiding this comment.
idéalement il faudrait un role="toolbar" comme dans l'exemple ProseMirror. La navigation clavier est aussi différente (flèches pour naviguer entre les boutons, et quand on clique dessus ça remet le focus dans l'éditeur).
| disabled={disabled} | ||
| size="small" | ||
| variant={isMarkActive(editorState, strongMark) ? 'filled' : 'ghost'} | ||
| onMouseDown={event => { |
There was a problem hiding this comment.
- utiliser onClick pour supporter le clavier, et qui est déclenché quand on relâche le clic. C'est plus courant que de faire l'action au MouseDown, avant d'avoir relâché
- pourquoi event.preventDefault() ?
- il faut un label explicite aux boutons, là c'est le nom de l'icône ("BoldIcon"). Je vois qu'on utilise par endroits des fichiers de traduction en.ts, tu pourrais faire pareil en attendant qu'on trouve une meilleure solution
There was a problem hiding this comment.
- Utiliser onMouseDown permet runCommand avant le changement de focus
- L'event.preventDefault permet d'empecher le comportement par defaut d'un clic sur un bouton qui est de prendre le focus
- Je rajoute les labels
There was a problem hiding this comment.
dans la démo de prosemirror on peut cliquer sur les boutons de la toolbar au clavier et à la souris, et le champ garde le focus. Est-ce qu'ils donnent le code de ça pour faire pareil ? C'est un peu gênant de pouvoir focus les boutons au clavier mais qu'on peut pas cliquer dessus
| return ( | ||
| <Stack gap="0.5"> | ||
| {label ? ( | ||
| <Label htmlFor={id ?? localId} required={required}> |
There was a problem hiding this comment.
2 issues :
- the localId is not used on the editor so the label points to nothing
- a
labelelement cannot be linked to a div. You can use an id on the label andaria-labelledbyon the editor instead
| }), | ||
| className, | ||
| )} | ||
| data-disabled={disabled ? 'true' : undefined} |
There was a problem hiding this comment.
why do you need this data-disabled attribute ?
There was a problem hiding this comment.
Effectivement il n'est pas utile
| disabled?: boolean | ||
| sentiment?: 'danger' | 'success' | 'neutral' | ||
| }) => ( | ||
| <Row gap="1" templateColumns="minmax(0, 1fr) min-content"> |
There was a problem hiding this comment.
the container of the Notice message should have:
- an id paired with an
aria-describedbyattribute on the text editor to link the message to the editor - a role="status" which implicitely add an aria-live="polite" so that screen readers read the status message when it's updated
6e9a1dc to
63e7331
Compare
| } | ||
| await userEvent.click(doc) | ||
| await userEvent.type(doc, 'This is an example') | ||
| await userEvent.click(screen.getByText('Submit')) |
There was a problem hiding this comment.
always use getByRole if possible. This is more precise, because you want to click on a button, not a text
| await userEvent.click(screen.getByText('Submit')) | |
| await userEvent.click(screen.getByRole('button', { name: 'Submit' })) |
| const italicButton = screen.getByTitle('ItalicIcon').closest('button') | ||
| const bulletListButton = screen | ||
| .getByTitle('ListBulletIcon') | ||
| .closest('button') |
There was a problem hiding this comment.
| const italicButton = screen.getByTitle('ItalicIcon').closest('button') | |
| const bulletListButton = screen | |
| .getByTitle('ListBulletIcon') | |
| .closest('button') | |
| const italicButton = screen.getByRole('button', { name: "Italic" }) | |
| const bulletListButton = screen | |
| .getByRole('button', { name: "Bullet List" }) |
| await userEvent.type(doc, 'Styled ') | ||
| await userEvent.click(bulletListButton!) | ||
| await userEvent.type(doc, 'item') | ||
| await userEvent.click(screen.getByText('Submit')) |
There was a problem hiding this comment.
| await userEvent.click(screen.getByText('Submit')) | |
| await userEvent.click(screen.getByRole('button', { name: "Submit" })) |
| const status = screen.getByRole('status') | ||
| expect(status).toHaveAttribute('aria-describedby', 'id-test-notice') | ||
| expect(status).toHaveTextContent(successMessage) |
There was a problem hiding this comment.
it's not the status that should have the aria-describedby attribute, it's the element that is described by the status, so in your case the text editor. You have to :
- add an id attribute on the status element
- add an aria-describedby="status-id" attribute on the editor
this way you "link" the status to the editor, and screen readers will read the status when focusing the editor.
As suggested, you can use toHaveAccessibleDescription to check this behavior without reading directly the attributes (test the behavior and not the implementation)
| const status = screen.getByRole('status') | |
| expect(status).toHaveAttribute('aria-describedby', 'id-test-notice') | |
| expect(status).toHaveTextContent(successMessage) | |
| const doc = screen.getByLabelText<HTMLDivElement>('Test') | |
| expect(doc).toHaveAccessibleDescription(successMessage) | |
| expect(screen.getByText(successMessage)).toHaveRole('status') |
use this same syntax for the other tests below
| }), | ||
| className, | ||
| )} | ||
| data-disabled={disabled ? 'true' : undefined} |
| onFocus={onFocus} | ||
| /> | ||
| {error ? ( | ||
| <AlertCircleIcon |
There was a problem hiding this comment.
The icon seems to me you're in the right place, we can see together if you can reproduce it.
| return ( | ||
| <Stack gap="0.5"> | ||
| {label ? ( | ||
| <Label htmlFor={id} id={`${id}-label`} required={required}> |
There was a problem hiding this comment.
you have to generate an id with useId() as a fallback if it's not provided in the props, otherwise you have "undefined" ids in the DOM
| disabled={disabled} | ||
| size="small" | ||
| variant={isMarkActive(editorState, strongMark) ? 'filled' : 'ghost'} | ||
| onMouseDown={event => { |
There was a problem hiding this comment.
dans la démo de prosemirror on peut cliquer sur les boutons de la toolbar au clavier et à la souris, et le champ garde le focus. Est-ce qu'ils donnent le code de ça pour faire pareil ? C'est un peu gênant de pouvoir focus les boutons au clavier mais qu'on peut pas cliquer dessus
| /> | ||
|
|
||
| <RichTextInput | ||
| aria-label="Bulleted list" |
There was a problem hiding this comment.
you should not need the aria-label in all those examples if it's the same as the label. I think the aria-label on this component should only be used in rare cases like when we don't want the label to be visible, we use the aria-label instead


Summary
Type
Summarize concisely:
What is expected?
RichTextInput: create componentRichTextInputandRichTextInputFieldThe following changes were made:
(Describe what you did)
Relevant logs and/or screenshots